feat: webrtc by default if supported #793

daviddias · 2017-03-15T18:20:00Z

I want to add this in, but cope with the scenarios where there is no webrtc-support. The current way does it, but also it makes it annoying to disable webrtc only, you'd have to swap all the multiaddrs at the same time to stop supporting webrtc. Will look into a better way.

dignifiedquire · 2017-03-15T19:07:57Z

src/core/components/init.js

@@ -45,6 +46,12 @@ module.exports = function init (self) {
        opts.log('done')
        opts.log('peer identity: ' + config.Identity.PeerID)

+        if (webrtcSupport.support || isNode) {


isNode does not gurantee webrtc support

I take suggestions.

try { require('webrtc') } catch () { console.log('sad') }

You mean wrtc?

daviddias · 2017-05-10T14:14:49Z

Reviewing this, the hard case is when a user wants to say "no I don't want WebRTC".

On a second thought, we actually can just make it part of the default multiaddrs, swarm will know how to handle non existing transports for multiaddrs to listen. This is a far simpler solution and it makes total sense with the new init \o/

daviddias · 2017-05-10T15:02:57Z

All right, this is ready :)

daviddias · 2017-05-10T15:20:51Z

Travis fails due to #844, not related with this PR

daviddias · 2017-05-10T15:38:04Z

Tested on Linux manually too:

daviddias · 2017-05-10T16:17:08Z

Ok, after experimenting a bit with it in Linux, I believe I found a more sane approach until we have a WebRTC transport that is rock solid in Linux and Windows.

@victorbjelkholm and all the Linux users of js-ipfs, mind giving your feedback on:

568479f

victorb · 2017-05-12T11:08:42Z

Not a big fan of turning off webrtc for linux users, but since it's not stable enough, this is probably the best way to go about it.

However, if I specify a libp2p-webrtc-star address in my own swarm config, and don't set USE_WRTC, I wouldn't expect it to remove them for me, I can see how that would be very confusing since I'm explicitly setting it myself.

If I don't, I agree we shouldn't automatically add it.

victorb · 2017-05-12T11:09:28Z

Ah, maybe include that you can use USE_WRTC to override, in the console.log

daviddias · 2017-05-12T11:41:00Z

From what I can read from your second point, it matches what is happening

USE_WRTC is only used during the init, this gives a way for every user to follow init -> daemon process without hitting any hiccup. For the more advanced user that modifies the IPFS config by hand, then that user will know better what they are doing and if they configured WRTC correctly, they will see it working.

Yeah, agree with you, wish there was a good WebRTC implementation.

victorb · 2017-05-12T11:50:05Z

@diasdavid hm, you're thinking about the CLI, and I think in that case, it makes sense.

However, when using it in code, the usage becomes a lot harder if I can't just specify it in my swarm addresses and it sticks. If instead, I need to run through the normal startup, then once it's ready, I have to do config.get, modify and then config.set, it becomes a lot uglier.

I'm want this (https://github.com/VictorBjelkholm/js-ipfs-in-the-browser/blob/master/spawn-node.js#L14-L20) to still work the same way as before, even for Linux users, since the Swarm addresses are manually specified.

daviddias · 2017-05-12T11:57:18Z

@victorbjelkholm

In the browser, you won't have to do anything, since browser is not a linux or windows env

In Node.js, using it programmatically, you just need to set process.env.USE_WRTC=1 before your init call, if and only if, you are in windows or linux.

You will continue to be able to do -- https://github.com/VictorBjelkholm/js-ipfs-in-the-browser/blob/master/spawn-node.js#L14-L20 -- as before

victorb · 2017-05-12T12:00:12Z

@diasdavid ay, of course! Duh. Still, if it was a script executed by nodejs, even if I explicitly set swarm address and not specify USE_WRTC, it'll be overwritten, correct?

daviddias · 2017-05-12T12:03:39Z

@victorbjelkholm yes, but then you just set that environment flag and you are golden.

Another thing we can add is an EXPERIMENTAL flag for "WRTC_LINUX_WINDOWS" so that you can pass that instead of having to do the process.env.USE_WRTC

And yes, I agree if we add that experimental flag, it should be named the same. WRTC_LINUX_WINDOWS makes more sense to me now.

victorb · 2017-05-12T14:03:30Z

@diasdavid

but then you just set that environment flag and you are golden

Yeah... My point is, if I explicitly set something in the config and run the script, I don't assume js-ipfs to make a choice for me. If I set those addresses explicitly, it's not my expectation that those will be removed automatically unless I set a env var.

However, if I don't specify them, it would make sense that we don't add them.

daviddias · 2017-05-12T14:09:06Z

I see a bunch of if for a lot of conditions that happen when you are a more savvy user. The goal to fulfill the expectations of our every day new users that want to spawn an instance in the browser without having to learn that it needs to add another multiaddr to see it getting connected to the other browser instances.

The case you talk about is for the user that needs to go through all the trouble of downloading and building wrtc by hand for Linux, at that state he will be more than familiar with the README and the case.

For all the other users, it will work.

Kubuxu · 2017-05-12T15:12:48Z

In go we try to prefix IPFS flags with IPFS_ so environment namespaces can't collide. You might want to consider it here.

Kubuxu · 2017-05-12T15:14:08Z

src/init-files/default-config-node.json

@@ -2,7 +2,8 @@
  "Addresses": {
    "Swarm": [
      "/ip4/0.0.0.0/tcp/4002",
-      "/ip4/127.0.0.1/tcp/4003/ws"
+      "/ip4/127.0.0.1/tcp/4003/ws",
+      "/libp2p-webrtc-star/dns4/star-signal.cloud.ipfs.team/wss"


It is swarm listen address list. Sure it should be here?

yep @Kubuxu, WebRTC references the signalling point, not a local server ;)

daviddias · 2017-05-12T20:19:32Z

good point @Kubuxu! changed the name of the env flag to be prefixed by IPFS_

daviddias added the status/in-progress In progress label Mar 15, 2017

dignifiedquire reviewed Mar 15, 2017

View reviewed changes

daviddias self-assigned this Apr 5, 2017

daviddias added status/ready Ready to be worked and removed status/in-progress In progress labels Apr 5, 2017

daviddias force-pushed the feat/webrtc-by-default branch 4 times, most recently from 154822b to 5e0123b Compare May 10, 2017 14:59

daviddias requested a review from victorb May 10, 2017 16:29

Kubuxu reviewed May 12, 2017

View reviewed changes

daviddias force-pushed the feat/webrtc-by-default branch from 0dae64c to ea16621 Compare May 12, 2017 20:30

feat: add WebRTC by default as a multiaddr

4ea1571

daviddias force-pushed the feat/webrtc-by-default branch from ea16621 to 4ea1571 Compare May 12, 2017 20:31

daviddias merged commit 8164472 into master May 12, 2017

daviddias deleted the feat/webrtc-by-default branch May 12, 2017 20:36

daviddias removed the status/ready Ready to be worked label May 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: webrtc by default if supported #793

feat: webrtc by default if supported #793

daviddias commented Mar 15, 2017

dignifiedquire Mar 15, 2017

daviddias Mar 17, 2017

dignifiedquire Mar 17, 2017 •

edited

Loading

daviddias Mar 17, 2017

dignifiedquire Mar 17, 2017

daviddias commented May 10, 2017 •

edited

Loading

daviddias commented May 10, 2017

daviddias commented May 10, 2017

daviddias commented May 10, 2017

daviddias commented May 10, 2017

victorb commented May 12, 2017

victorb commented May 12, 2017

daviddias commented May 12, 2017

victorb commented May 12, 2017

daviddias commented May 12, 2017

victorb commented May 12, 2017

daviddias commented May 12, 2017 •

edited

Loading

victorb commented May 12, 2017

daviddias commented May 12, 2017

Kubuxu commented May 12, 2017

Kubuxu May 12, 2017

daviddias May 12, 2017

daviddias commented May 12, 2017

feat: webrtc by default if supported #793

feat: webrtc by default if supported #793

Conversation

daviddias commented Mar 15, 2017

dignifiedquire Mar 15, 2017

Choose a reason for hiding this comment

daviddias Mar 17, 2017

Choose a reason for hiding this comment

dignifiedquire Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

daviddias Mar 17, 2017

Choose a reason for hiding this comment

dignifiedquire Mar 17, 2017

Choose a reason for hiding this comment

daviddias commented May 10, 2017 • edited Loading

daviddias commented May 10, 2017

daviddias commented May 10, 2017

daviddias commented May 10, 2017

daviddias commented May 10, 2017

victorb commented May 12, 2017

victorb commented May 12, 2017

daviddias commented May 12, 2017

victorb commented May 12, 2017

daviddias commented May 12, 2017

victorb commented May 12, 2017

daviddias commented May 12, 2017 • edited Loading

victorb commented May 12, 2017

daviddias commented May 12, 2017

Kubuxu commented May 12, 2017

Kubuxu May 12, 2017

Choose a reason for hiding this comment

daviddias May 12, 2017

Choose a reason for hiding this comment

daviddias commented May 12, 2017

dignifiedquire Mar 17, 2017 •

edited

Loading

daviddias commented May 10, 2017 •

edited

Loading

daviddias commented May 12, 2017 •

edited

Loading